Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add service migrations (POC) #524

Open
wants to merge 1 commit into
base: 3.4.x
Choose a base branch
from
Open

Add service migrations (POC) #524

wants to merge 1 commit into from

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Jan 21, 2024 •
edited
Loading

Inspired by #521 (comment)

This is a proposed implementation for registering migrations as services.

To get this working Symfony needs to be able to autoload migrations:

    "autoload": {
        "psr-4": {
            "App\\": "src/",
            "DoctrineMigrations\\": "migrations/"
        }
    },

Then, import the migrations as services:

    DoctrineMigrations\:
        resource: '../migrations/'

Of course, an alternative approach would be to move the migrations folder to src and change the namespace to App\Migrations.

Once that is done, all you need to do is override the constructor and inject the desired services.

final class Version20240121015756 extends AbstractMigration
{
    public function __construct(
        Connection              $connection,
        LoggerInterface         $logger,
        private RouterInterface $router,
    ) {
        parent::__construct($connection, $logger);
    }

    public function up(Schema $schema): void
    {
        dump($this->router);

        // ...
    }

    // ...
}

I haven't added any tests yet, as I was hoping to get some feedback on my approach first.

@connorhu
Copy link

connorhu commented Feb 3, 2024

Here you can find a similar but much simpler implementation. My solution works without extending the autoloader.
3.3.x...connorhu:DoctrineMigrationsBundle:3.3.x

@derrabus
Copy link
Member

derrabus commented Feb 4, 2024

This PR will require to register migrations as services, right? If we go down that road, why would I need them to be service subscribers? I mean, I could just inject all dependencies directly, couldn't I?

@HypeMC
Copy link
Contributor Author

HypeMC commented Feb 4, 2024

@derrabus The reason for my approach are the standard migration arguments: Connection $connection, LoggerInterface $logger. Connection would get autowired to the connection in Doctrine bundle, not the result of doctrine.migrations.dependency_factory::getConnection(), same goes for the logger. Perhaps I could use bindings instead 🤔 . I'll give it a try.

@HypeMC
Copy link
Contributor Author

HypeMC commented Feb 26, 2024

@derrabus I've updated the implementation to use bound arguments instead of service locators.

@greg0ire greg0ire closed this Mar 15, 2024
@greg0ire greg0ire reopened this Mar 15, 2024
@AlexOstrovsky
Copy link

@greg0ire any plan when this will be merged?

@greg0ire
Copy link
Member

greg0ire commented Apr 2, 2024

No

@AlexOstrovsky
Copy link

@greg0ire what is blocking this from beeing merged?

@greg0ire
Copy link
Member

greg0ire commented Apr 3, 2024 •
edited
Loading

For starters:

  • There are conflicts.
  • The CI should be green.
  • The conversation with @derrabus must be resolved.

@HypeMC

This comment was marked as resolved.

Sign in to view
@greg0ire

This comment was marked as resolved.

Sign in to view
@greg0ire
Copy link
Member

greg0ire commented Apr 3, 2024

I know it's just a POC for now, but since I'm asked what's blocking, I'd add that docs are probably necessary for this sort of thing.

@connorhu
Copy link

connorhu commented Jun 5, 2024 •
edited
Loading

I started testing this PR with real world code. You should work on making this unnecessary:

    DoctrineMigrations\:
        resource: '../migrations/'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HypeMC @connorhu @derrabus @AlexOstrovsky @greg0ire